Skip to content

Comments

v8: move process.binding('v8') to internalBinding#22288

Closed
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:v8-internal-binding
Closed

v8: move process.binding('v8') to internalBinding#22288
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:v8-internal-binding

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Aug 12, 2018

Refs: #22160

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 12, 2018
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 12, 2018

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/539/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Aug 12, 2018
@jasnell jasnell mentioned this pull request Aug 12, 2018
Closed
33 tasks
@BridgeAR BridgeAR requested a review from a team August 13, 2018 00:32
@BridgeAR
Copy link
Member

BridgeAR commented Aug 13, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/16410/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 13, 2018
jdalton
jdalton suggested changes Aug 13, 2018
View reviewed changes
Copy link
Member

@jdalton jdalton left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding removals until migration is ironed out

jdalton
jdalton suggested changes Aug 13, 2018
View reviewed changes
Copy link
Member

@jdalton jdalton left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[wrong button]

@jasnell
Copy link
Member Author

jasnell commented Aug 13, 2018

@jdalton ... The require('v8') and process.binding('v8') bindings are explicitly not covered by existing semver rules.

See https://nodejs.org/dist/latest-v10.x/docs/api/v8.html#v8_v8:

The APIs and implementation are subject to change at any time.

Therefore, this PR should not be blocked by the transition discussion.

@jdalton
Copy link
Member

jdalton commented Aug 13, 2018

Therefore, this PR should not be blocked by the transition discussion.

I'd dig it if we could get a rough wag at usage before skipping deprecation processes.

@jasnell
Copy link
Member Author

jasnell commented Aug 13, 2018

Technically the v8 module is not covered by the normal deprecation processes so they're not being skipped. However, once #22269 lands, I can add 'v8' to the whitelist for the time being.

@jasnell jasnell force-pushed the v8-internal-binding branch from 01a0230 to a2d45fb Compare August 15, 2018 02:26
@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

@jdalton ... added process.binding('v8') to the fallthrough whitelist

@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

New CI: https://ci.nodejs.org/job/node-test-pull-request/16462/

@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

ping @nodejs/tsc

@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16494/

@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2018

Another CI failure in linux-containered: https://ci.nodejs.org/job/node-test-commit-linux-containered/6330/

@jasnell jasnell force-pushed the v8-internal-binding branch 2 times, most recently from 8840799 to 9a0dcd0 Compare August 16, 2018 21:27
@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2018

Trying CI yet again: https://ci.nodejs.org/job/node-test-pull-request/16500/

@Trott
Copy link
Member

Trott commented Aug 17, 2018

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16509/

This was referenced Aug 17, 2018
Closed
Closed
@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

New CI: https://ci.nodejs.org/job/node-test-pull-request/16531/

@jasnell jasnell force-pushed the v8-internal-binding branch from 9a0dcd0 to e0a0987 Compare August 18, 2018 13:33
@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

Trying again: https://ci.nodejs.org/job/node-test-pull-request/16536/

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

Trying arm fanned again because of an unrelated failure... https://ci.nodejs.org/job/node-test-commit-arm-fanned/3012/

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018 •
edited
Loading

Arm... again: https://ci.nodejs.org/job/node-test-commit-arm-fanned/3023/

@jasnell jasnell force-pushed the v8-internal-binding branch from e0a0987 to c5a3d31 Compare August 18, 2018 20:13
@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

Full CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/16555/

@Trott
Copy link
Member

Trott commented Aug 18, 2018

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16560/

jasnell added a commit to jasnell/node that referenced this pull request Aug 18, 2018
@jasnell
v8: move process.binding('v8') to internalBinding
892932f 
PR-URL: nodejs#22288
Refs: nodejs#22160
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

Landed in 892932f

@jasnell jasnell closed this Aug 18, 2018
@joyeecheung joyeecheung mentioned this pull request Aug 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr approved these changes

+5 more reviewers

@jdalton jdalton jdalton approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@maclover7 maclover7 maclover7 approved these changes

@hiroppy hiroppy hiroppy approved these changes

@devsnek devsnek devsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.